Only allow one runner at a time per connection to the agent by the protocol design #773
Conversation
…r at a time per connection to the agent
jnm2
force-pushed
the
simplify_agent_communication
branch
from
2060698 to
4945ba7
Compare
jnm2
force-pushed
the
simplify_agent_communication
branch
from
4945ba7 to
729f6cc
Compare
| @@ -107,7 +121,7 @@ public bool Start() | |||
| return true; | |||
| } | |||
|
|
|||
| public override void Stop() | |||
| public void ShutDown() | |||
There was a problem hiding this comment.
Agents used Stop to indicate a distinction with ShutDown, which has a special meaning for Services. Services youse StartUp and ShutDown, whereas communications elements have used Start and Stop. Where a comm element is also a service, Start and Stop are typically called as part of StartUp and ShutDown. Having these as separate methods makes some unit-testing easier.
There was a problem hiding this comment.
I will say, I've never been aware of that design intent! 😅
I have no strong opinion on the final naming. Joseph - re: Charlie's comment about unit testing, do you think that will cause an issue, based on your final goal here?
There was a problem hiding this comment.
@ChrisMaddock Well, to put it differently, the intent is seen in the definition of IService, which has StartUp and Shutdown methods. There is no rule against using the same method names outside of the interface, but it's potentially confusing... or maybe only confusing to me. 😄
| /// </summary> | ||
| public interface ITestAgent | ||
| { | ||
| /// <summary> | ||
| /// Stops the agent, releasing any resources | ||
| /// </summary> | ||
| void Stop(); | ||
| void ShutDown(); |
There was a problem hiding this comment.
Others may not be confused, but I guess I find it confusing to see an interface that implements ShutDown even though it's not an IService and which implements it without also implementing StartUp.
| /// </summary> | ||
| ITestEngineRunner CreateRunner(TestPackage package); | ||
| TestEngineResult Load(TestPackage package); |
There was a problem hiding this comment.
Reading this, I feel the need to read all the code so as to see if all the methods of ITestEngineResult are present. They are, so why not just inherit from that interface?
| @@ -0,0 +1,11 @@ | |||
| #if NET20 | |||
There was a problem hiding this comment.
I'm not much liking the name of this class. 😄 Why not "Func.cs"?
jnm2
force-pushed
the
simplify_agent_communication
branch
from
729f6cc to
645a8a7
Compare
| @@ -107,7 +121,7 @@ public bool Start() | |||
| return true; | |||
| } | |||
|
|
|||
| public override void Stop() | |||
| public void ShutDown() | |||
There was a problem hiding this comment.
I will say, I've never been aware of that design intent! 😅
I have no strong opinion on the final naming. Joseph - re: Charlie's comment about unit testing, do you think that will cause an issue, based on your final goal here?
| /// <summary> | ||
| /// Unloads any loaded package. If none is loaded, the call is ignored. | ||
| /// </summary> | ||
| void Unload(); |
There was a problem hiding this comment.
I expect your working on a minimal-change-basis here, but before we move on from this project entirely, I think we should consider if ignoring an empty Unload call rather than throwing is the behaviour we want here. It feels inconsistent with Reload's behaviour below...
There was a problem hiding this comment.
@ChrisMaddock It could have been different, but it's public behavior. I don't think we should change what has been a safe call into one that throws.
UPDATE: Might not be public in this particular interface, but it is what we do in ITestRunner.Unload, which is public.
Closes #763
Contributes to #266
Review a commit at a time to follow the incremental changes.